-
Notifications
You must be signed in to change notification settings - Fork 174
Support itblock for Prism::Translation::Parser
#3481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support itblock for Prism::Translation::Parser
#3481
Conversation
ff8045e to
7e0e098
Compare
| @@ -0,0 +1,7 @@ | |||
| -> do it + 42 end | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is actually not really tested, only Parser33 is run in the test suite. I thought about it a bit before, and probably a separate directory for new syntax would make sense (or something along those lines).
You probably need to assert specifically against the ast anyways, as it only compares to the parser gem, and there is nothing to compare against in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I've added a test to check the :itblock node type, which is currently provided only by Prism::Translation::Parser. Since it is essentially an extension of the Parser gem, I've added it to parser_test.rb.
Additionally, I've implemented a mechanism to compare node compatibility with the Parser::Ruby34, but since the Parser gem doesn't yet support the it block parameter, it is currently skipped.
15475ac to
722a9c7
Compare
test/prism/ruby/parser_test.rb
Outdated
| } | ||
| end | ||
|
|
||
| def assert_prism_only_node(actual_ast, fixture_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's doubtful if parser will ever support 3.4 and this seems highly specific. It also only works because this is valid syntax to the parser gem, if some new syntax gets added you can't test this via this mechanism.
I think I would just add new specific tests for this syntax. If the parser gem does end up supporting it, then these test cases can just be imported. Something like this:
# Pseudo-code like
def test_it_syntax
parser = Prism:Parser::Translator::Ruby34.new
expected = s(:itblock, s(:begin, s(:itarg)))
assert_equal(expected, parser.tokenize(it_fixture)[0].to_sexp)
endThen, you can extend the existing it.txt (stuff in fixtures/whitequark is pulled from upstream) and explicitly assert against it. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is currently unclear how the Parser gem will support Ruby 3.4 and later, but I agree with the idea of focusing on the necessary test for now. And using S-expressions in Prism::Translation::Parser to test preceding syntax is a good idea, as it clarifies the expected syntax tree. I have updated the test accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thanks!
It might be a bit annoying to have to update the expectation when the fixture changes, inlining the code so the test is entirely self-contained might be worth a thought. But I think for now this is fine.
## Summary
`itblock` node is added to support the `it` block parameter syntax introduced in Ruby 3.4.
```console
$ ruby -Ilib -rprism -rprism/translation/parser34 -e 'buffer = Parser::Source::Buffer.new("path"); buffer.source = "proc { it }"; \
p Prism::Translation::Parser34.new.tokenize(buffer)[0]'
s(:itblock,
s(:send, nil, :proc), :it,
s(:lvar, :it))
```
This node design is similar to the `numblock` node, which was introduced for the numbered parameter syntax in Ruby 2.7.
```
$ ruby -Ilib -rprism -rprism/translation/parser34 -e 'buffer = Parser::Source::Buffer.new("path"); buffer.source = "proc { _1 }"; \
p Prism::Translation::Parser34.new.tokenize(buffer)[0]'
s(:numblock,
s(:send, nil, :proc), 1,
s(:lvar, :_1))
```
The difference is that while numbered parameters can have multiple parameters, the `it` block parameter syntax allows only a single parameter.
In Ruby 3.3, the conventional node prior to the `it` block parameter syntax is returned.
```console
$ ruby -Ilib -rprism -rprism/translation/parser33 -e 'buffer = Parser::Source::Buffer.new("path"); buffer.source = "proc { it }"; \
p Prism::Translation::Parser33.new.tokenize(buffer)[0]'
s(:block,
s(:send, nil, :proc),
s(:args),
s(:send, nil, :it))
```
## Development Note
The Parser gem does not yet support the `it` block parameter syntax. This is the first case where Prism's node design precedes that of the Parser gem.
When implementing whitequark/parser#962, this node design will need to be taken into consideration.
722a9c7 to
c141e14
Compare
## Summary `Prism::Translation::Parser35` has been started development for Ruby 3.5 (edge Ruby): ruby/prism#3346 At present, there is no timeline for Ruby 3.5 support in the Parser gem: whitequark/parser#1046 Given this, support for Ruby 3.5 will first be introduced in `Prism::Translation::Parser`. This can also serve as a trigger to facilitate user migration from `ParserEngine: parser_whitequark` to `ParserEngine: parser_prism`. As for Ruby 3.4, `Prism::Translation::Parser` is working on supporting the `it` syntax in ruby/prism#3481, but the Parser gem has not yet supported for it. However, whether to deprecate Ruby 3.4 in the Parser gem will be considered separately from this PR. Since Ruby 3.5 is a development version with minimal impact on users, while Ruby 3.4 is a stable release, they will be evaluated separately. ## Additional Information First, the default `parser_engine` for Ruby 3.5 is set to `parser_prism`. This default aligns with the current state of support, as the Parser gem is not expected to support Ruby 3.5. The more challenging decision is how to handle the default for Ruby 3.4. Ruby 3.4 will likely serve as a transition period between the Parser gem and Prism. While it is possible to set the default `parser_engine` to `parser_prism` for Ruby 3.4 as well, considering the potential unexpected incompatibilities in `Prism::Translation::Parser`, the default remains `parser_whitequark`. In any case, the primary use case is RuboCop, and what matters most is which value RuboCop chooses to pass. In other words, the key decision lies with RuboCop.
Summary
itblocknode is added to support theitblock parameter syntax introduced in Ruby 3.4.This node design is similar to the
numblocknode, which was introduced for the numbered parameter syntax in Ruby 2.7.The difference is that while numbered parameters can have multiple parameters, the
itblock parameter syntax allows only a single parameter.In Ruby 3.3, the conventional node prior to the
itblock parameter syntax is returned.Development Note
The Parser gem does not yet support the
itblock parameter syntax. This is the first case where Prism's node design precedes that of the Parser gem. When implementing whitequark/parser#962, this node design will need to be taken into consideration.cc @whitequark @iliabylich